aitools: add --output json to install - #6481
Conversation
Approval status: pending
|
583618b to
5dbbee3
Compare
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
Automated multi-reviewer pass (Isaac, Codex, and Claude reviewers, cross-checked against the code). The feature itself is sound and mirrors aitools list --output json. Inline comments below; a few findings that don't anchor to a changed line:
[nice to have] --path + --output json silently ignores JSON (cmd/aitools/install.go:124) — the --path block returns before the jsonMode check (:138) and render (:197), so install --path DIR --output json does a text dump with exit 0 and no JSON. --skills-only+--path is already rejected at :105; per CLAUDE.md's "reject incompatible inputs early" rule, either reject this combination or honor JSON in the dump path.
[nice to have] Coverage / convention gaps — (1) no .nextchanges/cli/ changelog fragment for this user-visible output mode (the analogous aitools list --output json, #5233, shipped one; not CI-blocking but expected by .agents/rules/changelog.md); (2) only unit tests were added — .agents/rules/testing.md prefers acceptance coverage for user-visible output, and the suite already exists at acceptance/experimental/aitools/skills/install/. An acceptance case would also have caught the stderr finding below, since it runs the real binary through root.Execute.
[nit] Help text (cmd/aitools/install.go:83) — the Long help / "Escape hatches" list doesn't mention --output json or its --scope+--agents requirement, so users hit the error only at runtime.
| // also print runErr as a text "Error:" line. Silence cobra's | ||
| // error/usage output; the non-zero exit still comes from returning | ||
| // runErr. | ||
| cmd.SilenceErrors = true |
There was a problem hiding this comment.
[blocking] SilenceErrors doesn't suppress the Error: line — the JSON contract is broken in production.
This repo's root prints errors itself: cmd/root/root.go:166 does fmt.Fprintf(cmd.ErrOrStderr(), "Error: %s\n", …), gated only on errors.Is(err, ErrAlreadyPrinted) (root already sets SilenceErrors: true at root.go:40, so cobra never prints on its own). So setting SilenceErrors here is a no-op, and a failed install … --output json writes the JSON to stdout and Error: … to stderr.
TestInstallOutputJSON passes only because it calls the leaf cmd.Execute(), which bypasses root.Execute.
Established fix — see cmd/environments/output.go:36-38: after the successful render, return root.ErrAlreadyPrinted on failure (non-zero exit, no text line) instead of relying on SilenceErrors. Requires importing github.com/databricks/cli/cmd/root. Please also add a test that goes through root.Execute so the stderr assertion is meaningful.
| if jsonMode { | ||
| if jerr := renderJSON(cmd.OutOrStdout(), buildInstallOutput(opts.Scope, outcomes, runErr)); jerr != nil { | ||
| // Rendering failed, so the JSON the caller parses is broken. | ||
| // Report the render error (which also drives the deferred |
There was a problem hiding this comment.
[nit] "which also drives the deferred telemetry" is inaccurate — the deferred logInstallEvent fires regardless of the returned error, so returning runErr here doesn't drive telemetry. Worth trimming the parenthetical.
| // --- Test helpers --- | ||
|
|
||
| // newTestInstallCmd builds the install command with the persistent --output flag | ||
| // the root command supplies in production, so root.OutputType does not panic when |
There was a problem hiding this comment.
[nit] "so root.OutputType does not panic" — install uses installOutputIsJSON, not root.OutputType; the persistent flag is actually needed so cobra can parse --output on a detached command. (Moot if the command switches to root.OutputType.)
0f9f04a to
af4bb2a
Compare
Add JSON output to `aitools install`, driven entirely by flags so the run is fully non-interactive: require --scope and --agents (erroring and naming the missing flags otherwise) so no scope prompt, agent picker, or confirm is shown. executePlan now returns a per-agent outcome (name, delivery, status, message) that the JSON payload lists. A top-level failure with no per-agent entry (e.g. a skills-group install failure) is surfaced in a top-level "error" field; per-agent failures stay in their agent entry and are not duplicated there (executePlan wraps them so the two are distinguishable). Once the JSON result is rendered, silence cobra's text "Error:"/usage output so a failure is not reported twice; the non-zero exit still comes from returning the run error. Share the indented-JSON encoder between install and list as renderJSON. Co-authored-by: Isaac <no-reply@databricks.com>
af4bb2a to
047ee54
Compare
The changelog validator now requires each fragment to be a single line starting with a `* ` bullet and ending with a period before the trailing PR link group. Reformat the install --output json fragment accordingly. Co-authored-by: Isaac <no-reply@databricks.com>
Integration test reportCommit: f05afd8
Top 6 slowest tests (at least 2 minutes):
|
--output json is meant to emit only the structured JSON document, but the
installer still wrote progress lines ("Using skills version", "Fetching
skills manifest...", "Installed N skills.") to stderr, so a consumer saw
non-JSON interleaved with the result. Mark the context quiet in JSON mode
and route those library messages through cmdio.LogProgress, which respects
it. Text mode is unaffected. Update the acceptance test to use the
non-deprecated `aitools install` and assert only JSON is emitted.
Co-authored-by: Isaac <no-reply@databricks.com>
3 participants
Changes
Add
--output jsontoaitools install. This requires--scopeand--agentsto avoid needing to prompt users on stdoutWhy
Needed for #6482
Tests
Added unit tests